Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSRF Support for SameSite #7537

Closed
rwinch opened this issue Oct 17, 2019 · 29 comments
Closed

CSRF Support for SameSite #7537

rwinch opened this issue Oct 17, 2019 · 29 comments
Assignees
Labels
in: web An issue in web modules (web, webmvc) status: invalid An issue that we don't feel is valid type: enhancement A general enhancement
Milestone

Comments

@rwinch
Copy link
Member

rwinch commented Oct 17, 2019

Summary

An alternative to using the synchronizer token pattern is to use the approach described in https://tools.ietf.org/html/draft-west-first-party-cookies-07#section-5.2

Setting the "SameSite" attribute in "strict" mode provides robust
defense in depth against CSRF attacks, but has the potential to
confuse users unless sites' developers carefully ensure that their
session management systems deal reasonably well with top-level
navigations.

Consider the scenario in which a user reads their email at MegaCorp
Inc's webmail provider "https://example.com/". They might expect
that clicking on an emailed link to "https://projects.com/secret/
project" would show them the secret project that they're authorized
to see, but if "projects.com" has marked their session cookies as
"SameSite", then this cross-site navigation won't send them along
with the request. "projects.com" will render a 404 error to avoid
leaking secret information, and the user will be quite confused.

Developers can avoid this confusion by adopting a session management
system that relies on not one, but two cookies: one conceptualy
granting "read" access, another granting "write" access. The latter
could be marked as "SameSite", and its absence would provide a
reauthentication step before executing any non-idempotent action.
The former could drop the "SameSite" attribute entirely, or choose
the "Lax" version of enforcement, in order to allow users access to
data via top-level navigation.

@rwinch rwinch added in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement labels Oct 17, 2019
@rwinch rwinch added this to the 5.3.x milestone Oct 17, 2019
@eleftherias
Copy link
Contributor

@rwinch is this similar to gh-4341, or is it about replacing the synchronizer token pattern we are currently using?

@rwinch
Copy link
Member Author

rwinch commented Oct 17, 2019

It is different. We want to persist the expected CSRF token in a cookie marked with SameSite strict. If the cookie is present in supported browser, then we know that the request originated from the same site.

@phalt
Copy link

phalt commented Mar 5, 2020

Hey folks. This is marked for 5.3.0 milestone, but I don't see it in the release notes.
We have started to encounter an issue with SameSite and we want to be prepared for the wider release in future browsers - is this being implemented soon?

@samuelstein
Copy link

An easy way around this problem is to use org.springframework.http.ResponseCookie class. The class has a nice builder with samesite property.
Then you can add the cookie to the response header:
response.addHeader(HttpHeaders.SET_COOKIE, ResponseCookie.from(name, value).build().toString());

@rwinch
Copy link
Member Author

rwinch commented Mar 16, 2020

@phalt It's probably not obvious, but the the .x milestones are buckets of items that we would like to get into a release, but haven't planned capacity for it. That means this has not been concretely planned for a release. I have also updated the milestone description to be more informative.

@samuelstein
Copy link

thanks @rwinch. i can understand but google chrome is pushing hard to enforce this.

@rwinch
Copy link
Member Author

rwinch commented May 18, 2020

Would someone be interested in submitting a pull request using ResponseCookie?

@codecracker2014
Copy link
Contributor

I would like to take this one. I think this should be fixed for all cookies, also value should be configurable.

@rwinch
Copy link
Member Author

rwinch commented Jun 9, 2020

Thank you @codecracker2014 This issue is yours. I'd suggest creating separate tickets/PRs for any other places that need updated.

@tokuhirom
Copy link

@codecracker2014 any updates on this?

@codecracker2014
Copy link
Contributor

I'm sorry for late update on this. It's very good idea to use same-site attribute to implement CSRF protection, but the problem is setter for sameSite is not available in Servlet API(javax.servlet.http.Cookie). refer https://wiki.shibboleth.net/confluence/display/DEV/Tomcat+and+Jetty+SameSite+Workarounds
Should i use setHeader to set this cookie?
ex. response.setHeader("Set-Cookie", "key=value; HttpOnly; SameSite=strict")

@codecracker2014
Copy link
Contributor

Any suggestions on Servlet API dependency? Servlet API 5.0.* yet to be released with support of SameSite, we should wait for that release to implement this feature.

@shartte
Copy link

shartte commented Apr 25, 2021

Keep in mind that for intranet applications, relying on SameSite=Strict for CSRF protection is not sufficient.
same site is not same origin!

Imagine a company hosting its intranet sites under company.com:

If application A uses a session cookie with SameSite=Strict under a.company.com, another application B under b.company.com is still considered to be same site, and application A will be CSRF vulnerable against application B (which might have a different security profile, or might be vulnerable).

Read this for an explanation: https://web.dev/same-site-same-origin/

@jzheaux
Copy link
Contributor

jzheaux commented May 6, 2021

Should i use setHeader to set this cookie?
ex. response.setHeader("Set-Cookie", "key=value; HttpOnly; SameSite=strict")

Yes, @codecracker2014, I think you would use the suggestion outlined here.

we should wait for that release to implement this feature.

I wonder if want to wait that long as Spring Security will not move its baseline from Servlet 3.1 until a major release. It would be nice to have this feature in the Spring Security 5.x line.

@codecracker2014
Copy link
Contributor

I've created pull request #9843 for this issue.

@rwinch
Copy link
Member Author

rwinch commented Jun 25, 2021

Thanks @codecracker2014!

As you noticed this issue is pretty old and so I wanted to refresh my understanding around CSRF best practices. At the moment, I'm wondering if it is a good idea to replace Synchronizer Token Pattern as we are doing here because the OWASP CSRF Cheat Sheet says that SameSite should only be used as a defense in depth mechanism, despite (as I understand it) the RFC stating that SameSite provides "robust protection". The downside, of course, to only using SameSite is that older browsers will not be protected. However, we can document that and perhaps even check the agent header to determine if this protection is enough (rejecting requests from older browsers).

I've reached out to OWASP on the current guidance around if SameSite can be used as a replacement to Synchronizer Token Pattern as it isn't clear to me this is considered safe. See OWASP/CheatSheetSeries#691

From here we will see what the response on the ticket I created is and take it from there.

@shartte
Copy link

shartte commented Jun 25, 2021

@rwinch SameSite does not protect against CSRF within larger intranet spaces that share the same ETLD+1.
Example: intranetapp1.company.com and intranetapp2.company.com share the same ETLD+1 and thus even with SameSite=strict, CSRF would be possible from one to the other.

@codecracker2014
Copy link
Contributor

codecracker2014 commented Jun 28, 2021

OWASP is recommending to implement SameSite cookie along with Synchronizer token (OWASP cheat sheet) to provide additional layer of defense. SameSite cookie can enable us to control top level navigation. Like some sites might disallow user to continue if page opened from third party site.

@rwinch Instead of replacing Synchronizer token we can implement provision to add SameSite attribute to session cookie this will provide additional defense.

@rwinch
Copy link
Member Author

rwinch commented Nov 17, 2021

After quite a bit of deliberation and feedback on the owasp ticket, I've decided to close this as won't fix (at least for now). The problem is that using SameSite is still really a defense in depth mechanism and does not solve for every cases. For example, it does not protect subdomains or older browsers (yes old browsers are very prevalent in many companies still). Frameworks should make it difficult (if not impossible) to make insecure choices. Until SameSite is established as a complete solution to CSRF attacks, we should not allow users to replace tokenizer pattern with SameSite.

@rwinch rwinch closed this as completed Nov 17, 2021
@rwinch rwinch self-assigned this Nov 17, 2021
@rwinch rwinch added the status: invalid An issue that we don't feel is valid label Nov 17, 2021
@ugrave
Copy link
Contributor

ugrave commented Jul 27, 2022

Hi,

im confused about reason why setting SameSite and CSRF coockie was rejected.

Now most browsers use lax as default if SameSite is not set.
But preffered way is to explicit set the same site value.

This should be an additional configuration and not used as a replacment of the sync token pattern.

Currently i used custom token repo which delegates to the original spring coockie repro and adds the SameSite attribute to the cockie. Better would be to have already support in the existing repro which use Lax as default.

@BenICE
Copy link

BenICE commented Aug 3, 2022

I also think in this ticket two topics are mixed.

  1. Adding the possiblity to add a SameSite attribute to the existing cookie / mechanism
  2. Replacing the synchronizer token by pure session cookie (with SameSite)

As requested in #7990 and many in this topic discussed the first topic...so being able to set the SameSite attribute for the currently existing cookie (nothing else).

Yes, modern browsers fall back to SameSite=Lax, but in security I always prefer explicit setting over implicit assumptions.

@rwinch
Copy link
Member Author

rwinch commented Aug 8, 2022

The reasons for rejecting are outlined in link I provided in #7537 (comment)

If OWASP is not recommending an approach, it is very unlikely that we will implement it in Spring Security.

@BenICE
Copy link

BenICE commented Aug 8, 2022

@rwinch thanks for answering. But I believe we still not talking about the same thing.

I (and I believe most of the people here) do NOT want to replace CSRF protection mechanism by soley using the SameSite attribute.

We just want to be able to set the SameSite attribute (which ALL cookies have) for the CSRF cookie. So enhancing the CookieCsrfTokenRepository to allow to set the SameSite attribute.

This will not change any security related stuff for the worse -> right now the XSRF-TOKEN cookie will just fall back to SameSite=Lax as this is currently the standard by modern browser.

@anders-g-hansen
Copy link

@rwinch I would echo the point that @BenICE raises. I understand that setting SameSite=Strict will not replace CSRF cookie protection. We are just looking to close the gap some penetration test reports raise where they expect all cookies produced from a site to be SameSite=Strict. Since CookieCsrfTokenRepository is final the only way to accomplish this today is to re-implement the entire class. I know there are other options for ensuring all cookies are set to strict via Tomcat/Http Server configurations, but would rather take a deployment independent approach and have it configured on CookieCsrfTokenRepository directly.

@OlliL
Copy link

OlliL commented Feb 17, 2023

For those who are just looking for a way to set the SameSite Attribute for the CSRF-Cookie:

I just worked around that issue by putting the following Bean into my App-Context... this adds SameSite Lax (check: org.springframework.boot.web.embedded.undertow.UndertowServletWebServerFactory.SuppliedSameSiteCookieHandler.beforeCommit(HttpServerExchange) to see where its evaluated for an Undertow setup)

  @Bean
  public CookieSameSiteSupplier applicationCookieSameSiteSupplier() {
    return CookieSameSiteSupplier.ofLax().whenHasName("XSRF-TOKEN");
  }

That should work for Undertow, Tomcat and Jetty as far as I can see.

@svschouw-bb
Copy link
Contributor

svschouw-bb commented Feb 20, 2023

For those who are just looking for a way to set the SameSite Attribute for the CSRF-Cookie:

I just worked around that issue by putting the following Bean into my App-Context... this adds SameSite Lax (check: org.springframework.boot.web.embedded.undertow.UndertowServletWebServerFactory.SuppliedSameSiteCookieHandler.beforeCommit(HttpServerExchange) to see where its evaluated for an Undertow setup)

  @Bean
  public CookieSameSiteSupplier applicationCookieSameSiteSupplier() {
    return CookieSameSiteSupplier.ofLax().whenHasName("XSRF-TOKEN");
  }

That should work for Undertow, Tomcat and Jetty as far as I can see.

This only works when using Spring Boot, and only when using an embedded webserver, not when building a WAR and deploying it to an existing webserver.

I think the same thing can be done by just putting server.servlet.session.cookie.same-site=LAX into application.properties.

Edit: sorry, got XSRF and SESSION cookies confused.

@OlliL
Copy link

OlliL commented Feb 20, 2023

I think the same thing can be done by just putting server.servlet.session.cookie.same-site=LAX into application.properties. But this only works when using Spring Boot, and only when using an embedded webserver, not when building a WAR and deploying it to an existing webserver.

Unfortunally not because thats only evaluated for the Spring Session Cookie, check org.springframework.boot.web.embedded.tomcat.TomcatServletWebServerFactory.configureCookieProcessor(Context) for example how its applied

@svschouw-bb
Copy link
Contributor

Unfortunally not because thats only evaluated for the Spring Session Cookie, check org.springframework.boot.web.embedded.tomcat.TomcatServletWebServerFactory.configureCookieProcessor(Context) for example how its applied

You're right, I keep getting those two confused (like everyone).

@arturCwiklinsky
Copy link

arturCwiklinsky commented Apr 16, 2024

Seems like

CookieCsrfTokenRepository repository = CookieCsrfTokenRepository.withHttpOnlyFalse();
repository.setCookieCustomizer(cookie -> {
            cookie.sameSite("Lax");
        });

works for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) status: invalid An issue that we don't feel is valid type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests